Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IEP-1301: Null Build error if you're building a project with out any esp-idf version #1062

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

alirana01
Copy link
Collaborator

@alirana01 alirana01 commented Oct 7, 2024

Description

Fixed the error message in case the toolchains are missing

Fixes # (IEP-1301)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested?

Please follow steps in Jira ticket

Test Configuration:

  • ESP-IDF Version: any
  • OS (Windows,Linux and macOS): all

Checklist

  • PR Self Reviewed
  • Applied Code formatting
  • Verified on all platforms - Windows,Linux and macOS

Summary by CodeRabbit

  • New Features

    • Enhanced error handling for toolchain retrieval in the build configuration.
    • Added a specific error message for missing toolchains to assist users in troubleshooting.
  • Bug Fixes

    • Improved validation process for project description IDF path against the environment variable IDF path.
  • Documentation

    • Updated messaging capabilities with a new localized error message for missing toolchains.

Sorry, something went wrong.

@alirana01 alirana01 self-assigned this Oct 7, 2024
@alirana01 alirana01 requested a review from sigmaaa October 7, 2024 11:18
Copy link

coderabbitai bot commented Oct 7, 2024

Walkthrough

The changes in this pull request involve modifications to the IDFBuildConfiguration and Messages classes within the com.espressif.idf.core.build package. Key updates include enhanced error handling for the toolchain file retrieval and validation checks in the build process. Additionally, a new static string for error messaging related to missing toolchains has been introduced, along with a corresponding entry in the messages.properties file. Minor formatting adjustments were also made to improve code clarity.

Changes

File Change Summary
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java - Added error handling in getToolChainFile() for null toolChain.
- Updated buildPrechecks() to check project description IDF path against environment variable IDF path.
- Minor formatting changes.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java - Added new public static string variable IDFToolChainsMissingErrorMsg for error messaging.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties - Added new entry IDFToolChainsMissingErrorMsg=Toolchains are missing. Please use ESP-IDF Manager for configuring.

Possibly related PRs

Suggested reviewers

  • AndriiFilippov
  • kolipakakondal
  • sigmaaa

Poem

🐰 In the meadow of code, we hop and play,
New checks and messages brighten the day.
Toolchains now guarded, no errors to find,
With each little change, our build's more refined!
So let’s celebrate, with a leap and a cheer,
For robust configurations, we hold so dear! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Collaborator Author

@alirana01 alirana01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self Review

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (1)

27-27: Improve grammar and clarity of the error message.

The new error message is a good addition, but it can be improved for better clarity and grammar.

Consider applying this change:

-IDFToolChainsMissingErrorMsg=Toolchains are missing please verify that idf tools are installed
+IDFToolChainsMissingErrorMsg=Toolchains are missing. Please verify that ESP-IDF tools are installed correctly.

This revision:

  1. Adds a period after "missing" to separate the two clauses.
  2. Capitalizes "ESP-IDF" for consistency with official naming.
  3. Adds "correctly" to emphasize proper installation.

These small changes will make the message more professional and potentially more helpful to users encountering this error.

bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)

283-288: LGTM: Improved error handling in getToolChainFile() method.

The addition of the null check for toolChain and the throwing of a CoreException with a specific error message greatly improves the error handling in this method. This change will help prevent NullPointerExceptions and provide more meaningful error messages.

One minor suggestion:

Consider extracting the error message to a constant in the Messages class for easier maintenance and reuse. For example:

public static final String TOOLCHAIN_MISSING_ERROR = "Toolchains are missing. Please check your project configuration.";

Then use it in the getToolChainFile() method:

throw new CoreException(new Status(IStatus.ERROR, IDFCorePlugin.PLUGIN_ID, Messages.TOOLCHAIN_MISSING_ERROR));

This approach would make it easier to update the error message in the future if needed.


398-399: Remove unnecessary empty lines.

There are two empty lines at the end of the buildPrechecks() method that don't serve any purpose. To maintain code cleanliness and consistency, it's recommended to remove these lines.

Consider removing the empty lines at the end of the buildPrechecks() method:

- 		
- 		

		return true;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between d8fe3e1 and 1a7a49f.

📒 Files selected for processing (3)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (3 hunks)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (1 hunks)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (2)

Line range hint 1-53: Summary: New error message variable added successfully.

The addition of the IDFToolChainsMissingErrorMsg variable is consistent with the existing code structure and supports the PR's objective of addressing the error message for missing toolchains. The change is minimal and doesn't introduce any apparent issues.

To further improve the PR:

  1. Ensure that the corresponding message text is added to the messages.properties file (as verified in the previous comment).
  2. Consider adding a brief comment above the new variable to describe its purpose, maintaining consistency with any existing documentation practices in the file.

35-35: LGTM: New message variable added correctly.

The new variable IDFToolChainsMissingErrorMsg has been added correctly, following the existing pattern for message declarations in this class. This addition aligns with the PR objective to address the error message for missing toolchains.

However, to ensure completeness:

Let's verify if the corresponding message has been added to the resource bundle:

This will help confirm that the message text has been properly defined in the resource bundle.

✅ Verification successful

Verification Successful: Message Key Present in Resource Bundle

The IDFToolChainsMissingErrorMsg key has been successfully added to the messages.properties file with the appropriate message. This ensures that the new message variable in Messages.java is properly linked to its corresponding message in the resource bundle.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the new message key exists in the properties file

# Test: Search for the new message key in the properties file
rg --type properties "IDFToolChainsMissingErrorMsg" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties

Length of output: 170


Script:

#!/bin/bash
# Description: Check if the new message key exists in the properties file

# Test: Search for the new message key in the properties file
rg "IDFToolChainsMissingErrorMsg" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties

Length of output: 212

bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (1)

27-27: LGTM: Change aligns well with PR objectives.

This addition of the error message for missing toolchains directly addresses the PR's objective of handling the error when toolchains are missing during the build process. The change is minimal, non-breaking, and consistent with the existing file structure, making it a suitable solution for the described issue.

To ensure this message key is being used correctly in the codebase, let's verify its usage:

This will help confirm that the new message is properly integrated into the error handling logic of the build system.

✅ Verification successful

Verification Successful: The error message key IDFToolChainsMissingErrorMsg is correctly integrated into the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the new error message key in the codebase.

# Test: Search for the usage of 'IDFToolChainsMissingErrorMsg'. Expect: At least one occurrence in a Java file.
rg --type java 'IDFToolChainsMissingErrorMsg'

Length of output: 564

bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (1)

69-72: LGTM: Import statements added correctly.

The new import statements for IStatus and Status are necessary for the changes in the getToolChainFile() method. These additions are correct and don't introduce any issues.

@AndriiFilippov
Copy link
Collaborator

AndriiFilippov commented Oct 10, 2024

@alirana01 hi !

The "Toolchains are missing please verify that idf tools are installed" message is duplicated.
Screenshot 2024-10-10 at 17 34 08

And I think the message offered by Kondal could be added to give user hint: Please use ESP-IDF Manager for configuring

@kolipakakondal kolipakakondal added this to the v3.1.1 milestone Oct 22, 2024
@alirana01
Copy link
Collaborator Author

@AndriiFilippov the error message that is duplicated is because of how the eclipse is propagating the exception messages back not something we can do. Further the error message suggestion was updated

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (1)

398-399: Remove unnecessary empty lines.

These extra empty lines create inconsistent spacing in the method.

-		
-		

		return true;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1a7a49f and 1ea7232.

📒 Files selected for processing (3)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (3 hunks)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (1 hunks)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties
🔇 Additional comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)

69-72: LGTM: Required imports added for error handling.

The new imports are necessary for the enhanced error handling implementation.


283-288: LGTM: Proper error handling for null toolchain.

The implementation correctly handles the case when toolchain is null, throwing a CoreException with a descriptive error message. This addresses the core issue mentioned in the PR objectives.

Let's verify that the error message is properly defined:

✅ Verification successful

Error message is properly defined and externalized

The error message constant IDFToolChainsMissingErrorMsg is correctly:

  • Defined in Messages.java as a public static field
  • Externalized in messages.properties with a clear user-friendly message
  • Used appropriately in the error handling code
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the error message constant is properly defined in Messages class
rg -A 1 "IDFToolChainsMissingErrorMsg" --type java

Length of output: 750

@AndriiFilippov
Copy link
Collaborator

AndriiFilippov commented Nov 4, 2024

@alirana01 hi !

Tested under:
OS - Windows 11
ESP-IDF: v5.3.1

LGTM, do see error message:
image

Question: I like the error message of "ESP-IDF" menu if the toolchains are missing. Do you think we could have same for "build" button ? or at least similar error message ?

image

@AndriiFilippov
Copy link
Collaborator

@alirana01

also this error message
image
I see only in case I delete esp-idf using Manager:
image

BUT if I go to "Preferences - > env" and delete toolchains from here:
image

and then try to build project - you get this message:
image

@alirana01
Copy link
Collaborator Author

@alirana01

also this error message ![image](https://private-user-images.githubusercontent.com/69584713/382659604-3b4cb086-c6bf-46c1-9f4d-59d12e1bde94.png?

I would suggest this to be separate ticket and i dont see any issue with this error message. We cannot try to override all the internal messages and how they are displayed I think this can be part of the patch release

@AndriiFilippov
Copy link
Collaborator

@sigmaaa @kolipakakondal please, review

Copy link
Collaborator

@sigmaaa sigmaaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@alirana01 alirana01 merged commit f0c20d6 into master Nov 19, 2024
2 of 4 checks passed
@alirana01 alirana01 deleted the IEP-1301 branch November 19, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants